Make SDK and EventSubscribe shutdown idempotent by Copilot · Pull Request #956 · FISCO-BCOS/java-sdk

Copilot · 2026-06-16T02:52:21Z

Stopping or destroying Client / EventSubscribe could re-enter native shutdown on the same JNI-backed handle and crash the JVM in bcos_sdk_stop. This change tightens Java-side lifecycle handling so repeated shutdown paths do not double-stop or tear down shared native state out of order.

Client lifecycle hardening
- Make ClientImpl.start(), stop(), and destroy() synchronized and stateful.
- Ignore repeated stop() / destroy() calls after the first effective shutdown.
- Prevent repeated native RpcJniObj.stop() / BcosSDKJniObj.destroy(...) on the same client handle.
Event subscription ownership model
- Distinguish between:
  - EventSubscribe.build(group, config) → owns the internally created client
  - EventSubscribe.build(client) → borrows the caller-provided client
- Only delegate shutdown/destruction to the client when the EventSubscribe instance owns it.
- Avoid directly destroying the JNI event wrapper when it aliases the client's native SDK handle.
Safer EventSubscribe shutdown
- Make EventSubscribeImp.stop() / destroy() idempotent.
- Unsubscribe active events before shutdown to reduce teardown races with in-flight native event state.
- Return an empty set when querying subscribed events after teardown instead of exposing null behavior.
Focused regression coverage
- Add unit tests covering repeated ClientImpl.stop() / destroy().
- Add unit tests covering shared-client vs owned-client EventSubscribe shutdown behavior.

Example of the intended behavior:

EventSubscribe sub = EventSubscribe.build(client);

sub.stop();
sub.stop();      // no-op

sub.destroy();
sub.destroy();   // no-op

* Initial plan * Remove fisco-bcos.org URL, replace with GitHub repo URL in build.gradle Co-authored-by: kyonRay <32325790+kyonRay@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: kyonRay <32325790+kyonRay@users.noreply.github.com>

…-sdk-stop

Copilot

Pull request overview

This PR hardens lifecycle management around JNI-backed SDK handles to prevent double-stop/double-destroy sequences (which can crash the JVM), and clarifies EventSubscribe ownership so it doesn’t tear down shared native state out of order.

Changes:

Make ClientImpl.start/stop/destroy synchronized and stateful to ensure shutdown is idempotent.
Introduce an ownership model in EventSubscribe/EventSubscribeImp so instances created via build(group, config) own and manage their internal client, while build(client) borrows without stopping/destroying it.
Add unit tests for repeated ClientImpl and EventSubscribeImp shutdown calls; update POM metadata URL.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/main/java/org/fisco/bcos/sdk/v3/client/ClientImpl.java`	Adds lifecycle state + synchronization to make start/stop/destroy idempotent.
`src/main/java/org/fisco/bcos/sdk/v3/eventsub/EventSubscribe.java`	Marks group-based `build(...)` as owning its internally created client.
`src/main/java/org/fisco/bcos/sdk/v3/eventsub/EventSubscribeImp.java`	Tracks ownership and makes stop/destroy idempotent; unsubscribes before teardown.
`src/test/java/org/fisco/bcos/sdk/v3/client/ClientImplTest.java`	Adds regression test for repeated stop/destroy (needs adjustment to avoid native teardown).
`src/test/java/org/fisco/bcos/sdk/v3/eventsub/EventSubscribeImpTest.java`	Adds regression tests for shared vs owned client shutdown (needs adjustment to avoid JNI init in unit tests).
`build.gradle`	Updates published project URL to the GitHub repository.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

    @Override
-    public void start() {
-        eventSubJniObj.start();
+    public synchronized void start() {
+        if (destroyed) {
+            return;
+        }
+        if (ownsClient) {
+            ownerClient.start();
+        }
+        stopped = false;
    }


+    private static void setField(Object target, String fieldName, Object value) throws Exception {
+        Field field = target.getClass().getDeclaredField(fieldName);
+        field.setAccessible(true);
+        field.set(target, value);
+    }
+}


+        client.stop();
+        client.stop();
+        client.destroy();
+        client.destroy();
+


+        Client client = mock(Client.class);
+        when(client.getGroup()).thenReturn("group0");
+        when(client.getNativePointer()).thenReturn(0L);
+
+        EventSubscribeImp eventSubscribe = new EventSubscribeImp(client, null);
+        EventSubJniObj eventSubJniObj = mock(EventSubJniObj.class);
+        when(eventSubJniObj.getAllSubscribedEvents())
+                .thenReturn(new HashSet<>(Arrays.asList("event-a", "event-b")));
+        setField(eventSubscribe, "eventSubJniObj", eventSubJniObj);


+        Client client = mock(Client.class);
+        when(client.getGroup()).thenReturn("group0");
+        when(client.getNativePointer()).thenReturn(0L);
+
+        EventSubscribeImp eventSubscribe = new EventSubscribeImp(client, null, true);
+        EventSubJniObj eventSubJniObj = mock(EventSubJniObj.class);
+        when(eventSubJniObj.getAllSubscribedEvents())
+                .thenReturn(new HashSet<>(Arrays.asList("event-a")));
+        setField(eventSubscribe, "eventSubJniObj", eventSubJniObj);


kyonRay · 2026-06-23T04:03:30Z

Review summary — core design is correct, a few things to fix before merge

✅ The core fix is sound. I verified that EventSubJniObj.build(long) does not allocate a separate native object — it only stores the passed-in pointer (setNativePointer). So eventSubJniObj.getNativePointer() == client.getNativePointer(): the event wrapper is a pure alias of the client's single native bcos_sdk handle. That confirms:

the old BcosSDKJniObj.destroy(eventSubJniObj.getNativePointer()) double-freed the same handle the client also frees → the SIGSEGV, so removing it is correct;
making ClientImpl.start()/stop()/destroy() idempotent (flag-guarded) and making EventSubscribe owner-vs-borrower aware is the right model.

ClientImpl is fine too: the constructor assigns rpcJniObj before calling start(), so started is set correctly after construction.

A few items to address, @copilot:

1. `EventSubscribe.start()` no longer activates the event channel (regression risk)

The crash you're fixing is in the stop/destroy path (double bcos_sdk_stop/destroy on the shared handle). start() (bcos_sdk_start) is not the crash path, so dropping its event-channel activation was unnecessary. As written, start() is a no-op unless ownsClient==true, which can break the documented build(client) → start() → subscribeEvent(...) flow.

Please restore activation in start(). Since ClientImpl.start() is now idempotent, the safe options are either:

delegate to ownerClient.start() in both owned and borrowed cases (idempotent, no double-start), or
keep calling eventSubJniObj.start() (this is the prior behavior on the already-started handle).

Keep stop()/destroy() owner-only — that part is correct; a borrowed client must not be torn down.

There is currently no test that asserts real event delivery. Please add an integration test that verifies an event is actually received after build(client).start() + subscribeEvent(...), and the same for build(group, config).

2. The new unit tests are JNI/native-dependent and fragile (very likely the Codacy failure)

EventSubscribeImpTest builds new EventSubscribeImp(client, null[, true]) via the real constructor, which calls the static EventSubJniObj.build(client.getNativePointer()) — a JNI call mockito-core cannot mock; it runs native init and is environment-dependent.
ClientImplTest.testStopAndDestroyAreIdempotent calls destroy() with a non-null mocked rpcJniObj, which executes the real static BcosSDKJniObj.destroy(rpcJniObj.getNativePointer()) native teardown.
Both rely on sun.misc.Unsafe.allocateInstance + reflective setAccessible, which the repo's Codacy ruleset flags.

Please allocate the instances without running the constructor (one shared helper) and inject the mocked eventSubJniObj / rpcJniObj and the ownership flags via reflection before invoking any lifecycle method, so that no real EventSubJniObj.build or BcosSDKJniObj.destroy native call executes. That makes the tests pure-Java and deterministic, and should clear the Codacy gate — please confirm the findings are resolved on the Codacy dashboard.

3. Drop the unrelated `build.gradle` change

The POM url change (http://www.fisco-bcos.org → the GitHub URL) is unrelated to shutdown idempotency. Please remove it from this PR to keep the changeset focused (a separate commit already moved the publish URL to central.sonatype).

Once 1–3 are addressed and Codacy is green, this is good to go. Thanks!

…-sdk-stop

sonarqubecloud · 2026-06-23T08:27:52Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

codecov · 2026-06-23T08:29:46Z

Codecov Report

❌ Patch coverage is 66.66667% with 15 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release-3.9.0@401fb58). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
.../fisco/bcos/sdk/v3/eventsub/EventSubscribeImp.java	70.58%	3 Missing and 7 partials ⚠️
.../java/org/fisco/bcos/sdk/v3/client/ClientImpl.java	60.00%	3 Missing and 1 partial ⚠️
...org/fisco/bcos/sdk/v3/eventsub/EventSubscribe.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             release-3.9.0     #956   +/-   ##
================================================
  Coverage                 ?   52.20%           
  Complexity               ?     4056           
================================================
  Files                    ?      430           
  Lines                    ?    17628           
  Branches                 ?     1969           
================================================
  Hits                     ?     9203           
  Misses                   ?     7684           
  Partials                 ?      741

Flag	Coverage Δ
unittest	`52.20% <66.66%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI and others added 2 commits March 4, 2026 13:34

Initial plan

f1e1c42

Copilot AI assigned Copilot and kyonRay Jun 16, 2026

Copilot started work on behalf of kyonRay June 16, 2026 03:37 View session

Fix idempotent SDK and eventsub shutdown

4b0e68b

Copilot AI changed the title ~~[WIP] Fix JVM SIGSEGV in native bcos_sdk_stop when stopping SDK~~ Make SDK and EventSubscribe shutdown idempotent Jun 16, 2026

Copilot finished work on behalf of kyonRay June 16, 2026 03:45

Copilot AI requested a review from kyonRay June 16, 2026 03:45

kyonRay changed the base branch from master to release-3.9.0 June 23, 2026 03:49

Merge branch 'release-3.9.0' into copilot/fix-jvm-sigsegv-native-bcos…

8c11a75

…-sdk-stop

kyonRay marked this pull request as ready for review June 23, 2026 03:53

Copilot AI review requested due to automatic review settings June 23, 2026 03:53

Copilot started reviewing on behalf of kyonRay June 23, 2026 03:53 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Copilot started work on behalf of kyonRay June 23, 2026 04:03 View session

Apply remaining changes

97cd2c0

Copilot finished work on behalf of kyonRay June 23, 2026 04:13

Merge branch 'release-3.9.0' into copilot/fix-jvm-sigsegv-native-bcos…

1c2c27d

…-sdk-stop

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make SDK and EventSubscribe shutdown idempotent#956

Make SDK and EventSubscribe shutdown idempotent#956
Copilot wants to merge 6 commits into
release-3.9.0from
copilot/fix-jvm-sigsegv-native-bcos-sdk-stop

Copilot AI commented Jun 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

kyonRay commented Jun 23, 2026

Uh oh!

sonarqubecloud Bot commented Jun 23, 2026

Uh oh!

codecov Bot commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Copilot AI commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

kyonRay commented Jun 23, 2026

Review summary — core design is correct, a few things to fix before merge

1. EventSubscribe.start() no longer activates the event channel (regression risk)

2. The new unit tests are JNI/native-dependent and fragile (very likely the Codacy failure)

3. Drop the unrelated build.gradle change

Uh oh!

sonarqubecloud Bot commented Jun 23, 2026

Quality Gate passed

Uh oh!

codecov Bot commented Jun 23, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Jun 16, 2026 •

edited

Loading

1. `EventSubscribe.start()` no longer activates the event channel (regression risk)

3. Drop the unrelated `build.gradle` change